The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enabling and validating the 'force inclusion' functionality within the system. It removes a previous block in the configuration validation that prevented the feature from being active and subsequently re-enables the comprehensive suite of end-to-end tests designed to verify its correct operation across various EVM components.

Highlights

• Force Inclusion Feature Activation: The 'forced inclusion' feature has been activated by removing a temporary error message that previously prevented its use in pkg/config/config.go. The configuration now correctly validates the ForcedInclusionNamespace.
• E2E Tests Re-enabled: All end-to-end tests related to EVM force inclusion, including sequencer, full node, and malicious sequencer scenarios, have been re-enabled in test/e2e/evm_force_inclusion_e2e_test.go by removing t.Skip() directives.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Claude finished @julienrbrt's task in 2m 31s —— View job

Code Review Complete ✅

I've completed a comprehensive code review of the force inclusion feature enablement. Here's my analysis:

Summary

This PR enables the force inclusion feature by:

1. Removing the validation blocker in pkg/config/config.go
2. Re-enabling 2 E2E tests (removing t.Skip())
3. Adding comprehensive benchmark tests for FilterTxs performance

The changes are straightforward and correctly implement the intended functionality. However, there are several issues that should be addressed before merging.

Critical Issues

1. Flag Prefix Inconsistency (pkg/config/config.go, test/e2e/evm_force_inclusion_e2e_test.go:352)

Issue: The code has inconsistent flag prefixes between evnode. and rollkit. in setupFullNodeWithForceInclusionCheck function.

Location: test/e2e/evm_force_inclusion_e2e_test.go:352-360

The full node setup uses rollkit.* flags:

"--rollkit.da.block_time", DefaultDABlockTime,
"--rollkit.da.address", endpoints.GetDAAddress(),
"--rollkit.da.namespace", DefaultDANamespace,
"--rollkit.da.forced_inclusion_namespace", "forced-inc",
"--rollkit.rpc.address", endpoints.GetFullNodeRPCListen(),
"--rollkit.p2p.listen_address", endpoints.GetFullNodeP2PAddress(),
"--rollkit.p2p.peers", fmt.Sprintf("%s@%s", sequencerID, sequencerP2PAddr),

While other tests in the same file and the sequencer setup use evnode.* flags.

Why this matters:

• According to pkg/config/config.go:21-23, there are TWO supported prefixes: FlagPrefixRollkit = "rollkit." and FlagPrefixEvnode = "evnode."
• The normalization function at line 344-350 converts rollkit. → evnode. for backward compatibility
• However, mixing both prefixes in the same file creates confusion and maintenance burden
• The project should standardize on evnode.* as the primary prefix (based on the constant naming and file context)

Recommendation: Standardize all flags to use evnode.* prefix for consistency. This was already noted in the review comment by @randygrok.

Fix flag inconsistency →

2. Missing Test Coverage (pkg/config/config.go:295-299)

Issue: The re-enabled validation logic for ForcedInclusionNamespace lacks test coverage.

Evidence from Codecov:

• 2 lines added are missing coverage (0% patch coverage)
• Lines 296-297 specifically: the validation error path

Why this matters:

• Config validation is critical security infrastructure
• Invalid namespace configurations could lead to runtime failures or security issues
• The validation uses validateNamespace() which calls Celestia's share.NewNamespaceFromBytes() - this error path should be tested

Recommendation: Add unit tests in pkg/config/config_test.go to cover:

// Test cases needed:
// 1. Valid forced inclusion namespace → should pass validation
// 2. Invalid namespace format → should return proper error
// 3. Empty namespace when configured → should pass (already handled by len check)

Add config tests →

Code Quality Observations

3. Benchmark Implementation Quality (execution/evm/filter_bench_test.go)

Positive aspects:

• ✅ Excellent benchmark coverage with multiple scenarios
• ✅ Tests the critical performance path (hasForceIncludedTransaction flag impact)
• ✅ Comprehensive test cases: normal txs, forced txs, mixed with invalid txs, gas limits, size limits
• ✅ Proper use of b.ResetTimer() and b.ReportAllocs() for accurate measurements
• ✅ Clear naming convention with benchName() helper for readable output
• ✅ Tests realistic scenarios (100, 1K, 10K transactions)

Minor improvements:

a) Benchmark documentation (line 116, 142):
The comments are good but could be more precise about performance expectations:

// Current:
// BenchmarkFilterTxs_OnlyNormalTxs benchmarks FilterTxs when hasForceIncludedTransaction is false.
// In this case, UnmarshalBinary is NOT called - mempool transactions are already validated.

// Suggested addition:
// Expected: O(n) with minimal allocations, ~100-200 ns/op per transaction

b) Helper function safety (line 36-78):
generateSignedTransaction() uses b.Fatalf() which is appropriate, but consider adding context to errors:

// Line 69: Instead of generic error
b.Fatalf("failed to sign transaction: %v", err)
// Consider:
b.Fatalf("failed to sign transaction for nonce %d: %v", nonce, err)

c) Test data realism (line 53):
The benchmark generates random 16-byte data, but real transactions vary more in size. Consider:

// Generate variable-length data to simulate realistic transaction sizes
dataSize := 16 + (i % 100) * 10  // Vary between 16-1016 bytes

Overall: The benchmark implementation is production-ready and follows Go best practices.

Security Analysis

4. Force Inclusion Security Properties

Positive security aspects:

• ✅ Namespace validation uses Celestia's official library (share.NewNamespaceFromBytes)
• ✅ E2E tests validate the full security model including malicious sequencer detection
• ✅ Proper separation of concerns: forced inclusion namespace is separate from normal namespace

Security considerations:

a) Input validation (pkg/config/config.go:295):

if len(c.DA.GetForcedInclusionNamespace()) > 0 {
    if err := validateNamespace(c.DA.GetForcedInclusionNamespace()); err != nil {
        return fmt.Errorf("could not validate forced inclusion namespace (%s): %w", c.DA.GetForcedInclusionNamespace(), err)
    }
}

The validation only runs if the namespace is non-empty. This is correct behavior - forced inclusion is optional. However:

• Ensure documentation clearly states forced inclusion is optional
• Consider logging a warning if running sequencer without forced inclusion configured (reduces censorship resistance)

b) Malicious sequencer detection (test/e2e/evm_force_inclusion_e2e_test.go:401):
The test TestEvmSyncerMaliciousSequencerForceInclusionE2E is currently skipped with TODO fix before merge. This is a critical security test that validates:

• Sync nodes detect when sequencer omits forced inclusion transactions
• The grace period mechanism works correctly
• Nodes stop syncing malicious chains

Recommendation: This test MUST be fixed and passing before merge, as it validates a core security property of the system.

Performance Considerations

5. FilterTxs Performance Impact

The benchmarks reveal the performance characteristics of enabling force inclusion:

Key findings (based on benchmark design):

• When hasForceIncludedTransaction=false: Fast path, no unmarshaling
• When hasForceIncludedTransaction=true: Must call UnmarshalBinary() on every transaction

Performance implications:

// BenchmarkFilterTxs_CompareUnmarshalOverhead (line 267-297)
// This benchmark directly measures the overhead

Expected overhead from unmarshaling:

• 1K transactions: ~100-300µs additional latency (O(n) with small constant)
• 10K transactions: ~1-3ms additional latency
• Memory: Additional allocations per transaction for types.Transaction objects

Optimization opportunities:

1. Current code (implied from test): Unmarshals ALL transactions when any forced tx exists
2. Better approach: Only unmarshal forced transactions if they can be identified upfront
3. Consider caching unmarshaled transactions if they're reused

However, these optimizations should be done in follow-up PRs based on actual benchmark results.

Testing & Reliability

6. E2E Test Coverage

Re-enabled tests:

1. ✅ TestEvmSequencerForceInclusionE2E - Tests sequencer receiving and including forced txs
2. ✅ TestEvmFullNodeForceInclusionE2E - Tests full node syncing forced txs from sequencer

Still disabled:
3. ❌ TestEvmSyncerMaliciousSequencerForceInclusionE2E - Critical security test (line 402)

Test quality observations:

a) Good practices:

• Tests use realistic timeouts with require.Eventually
• Proper cleanup with defer client.Close()
• Clear logging of test progression
• Tests validate both happy path and sync scenarios

b) Timing assumptions (line 167-172):

require.Eventually(t, func() bool {
    return evm.CheckTxIncluded(client, txForce.Hash())
}, 30*time.Second, 1*time.Second, "Forced inclusion transaction not included")

30-second timeout seems reasonable for epoch=2 (2 DA blocks @ 200ms = 400ms theoretical minimum). The generous timeout accounts for:

• DA submission delays
• Block production time
• Network propagation

c) Test helper organization:
The helpers (setupSequencerWithForceInclusion, setupFullNodeWithForceInclusionCheck) are well-structured and reusable.

Documentation & Developer Experience

7. Documentation Quality

Config documentation (pkg/config/config.go:189):

ForcedInclusionNamespace string `mapstructure:"forced_inclusion_namespace" yaml:"forced_inclusion_namespace" comment:"Namespace ID for forced inclusion transactions on the DA layer."`

The comment is adequate but could be more informative:

// Suggested improvement:
comment:"Namespace ID for forced inclusion transactions on the DA layer. When set, the node will monitor this namespace for transactions that must be included within the epoch window. Leave empty to disable forced inclusion."

Test documentation (test/e2e/evm_force_inclusion_e2e_test.go:366-400):
The malicious sequencer test has EXCELLENT documentation explaining:

• Test architecture
• Threat model
• Expected behavior
• Configuration details

This is exemplary documentation for complex security tests.

Architecture & Design

8. Configuration Architecture

The config changes are minimal and follow existing patterns:

Good design aspects:

• ✅ Uses existing validateNamespace() function for consistency
• ✅ Conditional validation (only validates if namespace is set)
• ✅ Follows error wrapping pattern with fmt.Errorf(..., %w)
• ✅ Integrates cleanly with existing DA configuration structure

Design consideration:
The Config.Validate() method is growing large (309 lines). Consider refactoring validation logic into smaller methods:

func (c *Config) validateDAConfig() error { ... }
func (c *Config) validateNodeConfig() error { ... }

However, this is a minor refactoring opportunity, not a blocker for this PR.

Recommendations Summary

Must Fix Before Merge

1. ✅ Fix flag prefix inconsistency - Change rollkit.* to evnode.* in setupFullNodeWithForceInclusionCheck
2. ⚠️ Fix and enable the malicious sequencer test - Critical security validation
3. ⚠️ Add config validation tests - Cover the new validation path (addresses codecov concern)

Should Fix (High Priority)

4. Document that forced inclusion is optional in config comments
5. Add logging/warning when sequencer runs without forced inclusion configured

Nice to Have (Lower Priority)

6. Enhance benchmark documentation with performance expectations
7. Consider variable transaction sizes in benchmarks for more realistic testing
8. Add helper error context in benchmark test helpers

Positive Highlights

🎉 What's done well:

• Clean, minimal changes that do exactly what's needed
• Excellent benchmark test coverage for performance validation
• Well-documented E2E tests with clear security model
• Proper namespace validation using official libraries
• Follows existing code patterns and conventions

Conclusion

This PR successfully enables the force inclusion feature with appropriate validation and testing. The implementation is solid, but there are 3 critical items that should be addressed before merge:

1. Flag consistency (already identified by reviewer @randygrok)
2. Malicious sequencer test enablement (currently TODO)
3. Config validation test coverage (addresses codecov gap)

The benchmark additions are excellent and will help track performance impact as the feature evolves.

Overall assessment: Approve after addressing the critical items above.

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.97%. Comparing base (2325ace) to head (36ce299).

@@            Coverage Diff             @@
##             main    #3005      +/-   ##
==========================================
- Coverage   57.98%   57.97%   -0.01%     
==========================================
  Files         110      110              
  Lines       10522    10523       +1     
==========================================
  Hits         6101     6101              
- Misses       3770     3771       +1     
  Partials      651      651              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.